demo_diff_drive_controller #70

BorgesJVT · 2021-02-22T02:11:25Z

Resolves #50

A diffbot folder was created similar to rrbot for URDF files;
A hardware in ros2_control_demo_hardware was created with position and velocity states and velocity command;
Launch file was added into ros2_control_demo_robot.

codecov-io · 2021-02-22T02:17:22Z

Codecov Report

Merging #70 (b8c518e) into master (620b703) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master     #70     +/-   ##
========================================
  Coverage    0.00%   0.00%             
========================================
  Files          22       2     -20     
  Lines        1562     149   -1413     
========================================
+ Misses       1562     149   -1413

Flag	Coverage Δ
unittests	`0.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 620b703...b8c518e. Read the comment docs.

destogl

Thanks for our contribution it looks very good. I have mostly nitpicks and a few clarification comments.

ros2_control_demo_hardware/include/ros2_control_demo_hardware/diffbot_system.hpp

ros2_control_demo_hardware/CMakeLists.txt

ros2_control_demo_hardware/include/ros2_control_demo_hardware/diffbot_system.hpp

ros2_control_demo_robot/description/diffbot/diffbot_system.ros2_control.xacro

ros2_control_demo_robot/description/diffbot_system.urdf.xacro

ros2_control_demo_robot/launch/diffbot_system.launch.py

destogl

One step further.

destogl · 2021-03-03T13:25:56Z

ros2_control_demo_hardware/include/ros2_control_demo_hardware/diffbot_system.hpp

+#include "hardware_interface/types/hardware_interface_status_values.hpp"
+#include "ros2_control_demo_hardware/visibility_control.h"
+
+using hardware_interface::return_type;


Can you please remove this using and then adjust everywhere return_type? It's fairly simple with repalce-all function.

ros2_control_demo_hardware/include/ros2_control_demo_hardware/diffbot_system.hpp

destogl · 2021-03-03T13:29:47Z

ros2_control_demo_hardware/src/diffbot_system.cpp

+
+  for (uint i = 0; i < hw_commands_.size(); i++) {
+    // Simulate DiffBot's movement
+    hw_states_[1] = hw_commands_[i] + (hw_states_[1] - hw_commands_[i]) / hw_slowdown_;


Does this slowdown make sense when using diff drive robot? I understand that your commands are velocities? you should here probably integrate to get position state and mabye create some artificial dynamics when velocity is changed. Does this make sense to you?

I think I didn't understand the meaning of slowdown parameter. I was assuming it was a 'time constant' for rrbot dynamics.
Yes, my commands are velocities. I've changed my dynamics equation to get the velocity state.

I am aware that is necessary to integrate to get position state but as long as I understood, since the read function is reading from the hardware, the hardware would provide those infos (including the position state already integrated).
So, I integrated hw_states_[0] += hw_states_[1] / 2; with 'update_rate = 2' just to exhibit the data in radians. Otherwise, Should I calculate the current and last time inside diffbot_system? What do you recommend?

ros2_control_demo_robot/controllers/diffbot_diff_drive_controller.yaml

ros2_control_demo_robot/launch/diffbot_system.launch.py

BorgesJVT · 2021-03-10T00:56:38Z

Linters is putting a linelength=140 which is getting unrecognized.

bmagyar

looks good in general, a few comments that could reduce the overall content of this PR

ros2_control_demo_robot/description/diffbot/diffbot.gazebo.xacro

ros2_control_demo_robot/description/diffbot_system.urdf.xacro

ros2_control_demo_robot/launch/diffbot_system.launch.py

bmagyar

Looks good to me

BorgesJVT · 2021-04-19T12:44:02Z

Friendly ping @destogl

destogl

I don't want to keep this open anymore. @BorgesJVT thank you for your patience. I will open a follow-up issue for the documenation. I will try to integrate this into #76.

Karsten1987 · 2021-04-28T19:21:50Z

This PR is not working correctly I believe. The controller config is not being installed, which leads to an incorrect loading of parameters and the ros2_control node eventually failing due to a non-existing update_rate parameter.

destogl mentioned this pull request Feb 22, 2021

Create demo for diff drive controller #50

Closed

destogl self-requested a review February 22, 2021 17:19

destogl requested changes Feb 22, 2021

View reviewed changes

BorgesJVT requested a review from destogl February 23, 2021 11:04

destogl reviewed Mar 3, 2021

View reviewed changes

BorgesJVT requested a review from destogl March 16, 2021 12:46

bmagyar requested changes Mar 31, 2021

View reviewed changes

BorgesJVT requested a review from bmagyar April 5, 2021 13:16

bmagyar approved these changes Apr 9, 2021

View reviewed changes

BorgesJVT and others added 10 commits April 28, 2021 10:26

demo_diff_drive_controller

288325a

fix ament_flake errors

324535e

applying review suggestions

cd92509

velocity and position models to simulate read hardware

94e2d9c

change folder to find packages

e9f74ee

Apply suggestions from code review

e16f38e

adjust everywhere return_type

b6d1d24

wheels first-order dynamics

2c531d9

remove slowdown parameter and uncrustify

bdaf0bd

applying review suggestions

c5badc8

destogl force-pushed the demo_diff_drive_controller branch from 883c7c3 to c5badc8 Compare April 28, 2021 08:34

Apply formatting

f6000a0

destogl approved these changes Apr 28, 2021

View reviewed changes

destogl mentioned this pull request Apr 28, 2021

Write documentation for dif drive example merged in #70 #87

Closed

destogl merged commit 1e14150 into ros-controls:master Apr 28, 2021

Karsten1987 mentioned this pull request Apr 28, 2021

make diff drive load correctly #89

Merged

demo_diff_drive_controller #70

demo_diff_drive_controller #70

Uh oh!

Conversation

BorgesJVT commented Feb 22, 2021

Uh oh!

codecov-io commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Uh oh!

destogl Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

destogl Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

BorgesJVT Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BorgesJVT commented Mar 10, 2021

Uh oh!

bmagyar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmagyar left a comment

Choose a reason for hiding this comment

Uh oh!

BorgesJVT commented Apr 19, 2021

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Uh oh!

Karsten1987 commented Apr 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Feb 22, 2021 •

edited

Loading